-
Notifications
You must be signed in to change notification settings - Fork 9.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[bitnami/zookeeper] Expose appProtocol, scheme, and tlsConfig for Istio compatibility #29683
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Jerod Culpepper <jcpepper@defenseunicorns.com>
Signed-off-by: Jerod Culpepper <jcpepper@defenseunicorns.com>
Signed-off-by: Jerod Culpepper <jcpepper@defenseunicorns.com>
Signed-off-by: Bitnami Containers <bitnami-bot@vmware.com>
Signed-off-by: Bitnami Containers <bitnami-bot@vmware.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for this contribution! Please take a look at my comments when you have a chance.
{{- if .Values.metrics.service.appProtocol }} | ||
appProtocol: {{ .Values.metrics.service.appProtocol }} | ||
{{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we simply rename the port name to http-metrics
?
@@ -28,4 +28,4 @@ maintainers: | |||
name: zookeeper | |||
sources: | |||
- https://github.com/bitnami/charts/tree/main/bitnami/zookeeper | |||
version: 13.4.14 | |||
version: 13.4.15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please bump minor version instead given you're adding new chart parameters
Description of the change
Hi folks, this PR is to expose the
appProtocol
field for the metrics Service and thescheme
andtlsConfig
fields for the ServiceMonitor.Benefits
This will enable Prometheus metrics to be configured properly when deploying ZooKeeper to a cluster configured with Istio. The metrics service port name is
tcp-metrics
causing Istio to automatically select the TCP protocol for this service which causes an error. By exposing theappProtocol
field we can override this selection with the correct protocol (in this case,http
).Similarly for the ServiceMonitor object, exposing the
scheme
andtlsConfig
will enable Prometheus metrics to be integrated with Istio.Possible drawbacks
I made the
appProtocol
,scheme
, andtlsConfig
values optional and empty by default to ensure backwards compatibility.Checklist
Chart.yaml
according to semver. This is not necessary when the changes only affect README.md files.README.md
using readme-generator-for-helm